-
Notifications
You must be signed in to change notification settings - Fork 196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: visually editable PDP #1808
feat: visually editable PDP #1808
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
78a9973
to
61386d0
Compare
61386d0
to
1044c5e
Compare
1044c5e
to
69ddc30
Compare
69ddc30
to
84742ae
Compare
624ef9d
to
0fd8e93
Compare
84742ae
to
203bfc2
Compare
203bfc2
to
b70653e
Compare
d4a99db
to
6983ee8
Compare
6983ee8
to
aa32df2
Compare
...props | ||
}: { | ||
type: string; | ||
label: string; | ||
label: string | Promise<string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allow label to be a promise so we can provide a descriptive label here.
Note that waiting for the label to resolve below feels a bit off as strictly speaking label is not required for rendering the snapshot element tree and could theoretically be resolved in parallel. Might be something we want to pursue later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be Streamable<string>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to make this dependent on Vibes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catalyst itself depends on VIBES so this shouldn't be an issue. We're also planning on creating a react-streamable
library and Catalyst will have it as it will be required to use with VIBES. Not a big deal, though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catalyst itself depends on VIBES so this shouldn't be an issue.
I know it does, but I view the collection of "userland" Makeswift boilerplate in this dir as a prototype for our updated integration guidelines, and I don't think they should depend on VIBES.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we ever want to make label a promise in that userland implementation?
I think we should; it's likely that giving a "dynamic" component a meaningful label would require a fetch.
If we did, wouldn't that effectively be introducing the streamable pattern
I personally don't think the label should be "streamed" here (awaited while delaying rendering/rendering a placeholder). I think it should be resolved in parallel and assigned to the snapshot's element tree asynchronously, independently of what happens in React.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should; it's likely that giving a "dynamic" component a meaningful label would require a fetch.
Given that the snapshot is not a promise, though, and that we await
label without a suspense boundary, there seems to be no benefit to taking label
as a promise. I think that taking in promises like this (i.e., the streamable pattern) is only relevant when the component uses Suspense
and handles the fallback appropriately. Simply making the prop a promise won't make it stream
I personally don't think the label should be "streamed" here (awaited while delaying rendering/rendering a placeholder). I think it should be resolved in parallel and assigned to the snapshot's element tree asynchronously, independently of what happens in React.
See my point above, accepting a promise here is useless since we await
it immediately and don't use Suspense
. The user can await
it themselves before passing it to us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@migueloller It's useless now, what I was proposing in my original comment is that, because label is not required for rendering the snapshot element tree and could theoretically be resolved in parallel, we make label an optional promise in the runtime and do resolve it in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you say "in parallel", do you mean "lazily" so that we do it at a later time and not in the path of rendering the component? For example, perhaps we resolve the promise right before we send the label via postMessage
to the builder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly.
aa32df2
to
99c7233
Compare
...props | ||
}: { | ||
type: string; | ||
label: string; | ||
label: string | Promise<string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be Streamable<string>
?
99c7233
to
85429b5
Compare
85429b5
to
ebabed3
Compare
What/Why?
Make product detail page visually editable.
Testing
Kapture.2025-01-08.at.13.42.10.mp4